Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Registered all namespaces in ModuleTemplateLoader class #9173

Merged
merged 5 commits into from Jun 12, 2018
Merged

Registered all namespaces in ModuleTemplateLoader class #9173

merged 5 commits into from Jun 12, 2018

Conversation

mickaelandrieu
Copy link
Contributor

@mickaelandrieu mickaelandrieu commented Jun 7, 2018

Questions Answers
Branch? 1.7.4.x
Description? We were unable to override each template of Back Office from a module because of only the namespace PrestaShop was registered.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
How to test? In any module already registered, create a new template called products_table.html.twig in modules/your-module/views/PrestaShop/Admin/Product/CatalogPage/Lists/products_table.html.twig with empty content. You should see no products anymore in your products catalog page

Important guidelines


This change is Reviewable

@prestonBot prestonBot added the 1.7.4.x Branch label Jun 7, 2018
@mickaelandrieu mickaelandrieu added the Bug Type: Bug label Jun 7, 2018
*/
public function setRegisteredPaths($registeredPaths)
{
dump($registeredPaths);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:'(

$templatePaths[] = $dir;
}
}
$this->setPaths($templatePaths, $namespace);
}
}

/**
* @return array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add this two functions? Where are they used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some tests, removed 👍

Copy link
Contributor

@PierreRambaud PierreRambaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

<tr><td colspan="11">
{% endblock %}
{% else %}
<tr><td colspan="11">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird indent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was here previously, improving it now may make the contribution unreadable

"icon": "remove_red_eye",
"label": "Preview"|trans({}, 'Admin.Actions')
}
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

{% if activate_drag_and_drop and has_category_filter %}class="sortable"{% endif %}
last_sql="{{ last_sql_query|escape('html_attr') }}"
{% if activate_drag_and_drop and has_category_filter %}class="sortable"{% endif %}
last_sql="{{ last_sql_query|escape('html_attr') }}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should be a data- attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope! we dont change anything here: not the scope of this contribution.

HALTE AU GRABUGE !

'Product': '/Admin/Product'
'Twig': '/Admin/TwigTemplateForm'
'AdvancedParameters': '/Admin/Configure/AdvancedParameters'
'ShopParameters': '/Admin/Configure/ShopParameters'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not store this list elsewhere, like in a config or something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used just once, not sure it's necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because this won't be re-used.

At start, I wanted to re-use the paths used in framework configuration, but thinking about it it's not a good idea. The 'array' look like the same but they have really different purposes.

If we have another place/service when we want to re-use it, of course I'm also in favor of extracting it into a parameter.

Note you can access to all registered Twig paths and namespaces using php bin/console debug:twig but not us, cause of weird call of Legacy Context in a Twig extension. This is a really minor issue we should try to fix asap.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we'll need to add new namespaces as we migrate new sections, won't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, so we will append new namespaces here.

But before I need to document which namespaces should be added as there is no real consistency actually: some templates use them, others don't, some namespace refers to a page, others to sections or even categories...

Once we will have consistency, I'm pretty sure we won't even need to configure an Array, but maybe loop into actual folders and declare namespaces dynamically ^^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it isn't done in runtime...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array
nope, it's converted in PHP function in container cache: I don't think we can make the process faster 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes everything setting in yml files are automatically converted in string / array in the cache file =)

PierreRambaud
PierreRambaud previously approved these changes Jun 7, 2018
@eternoendless eternoendless added this to the 1.7.4.0 milestone Jun 8, 2018
@mickaelandrieu mickaelandrieu added the Waiting for QA Status: action required, waiting for test feedback label Jun 9, 2018
PierreRambaud
PierreRambaud previously approved these changes Jun 11, 2018
@prasanthSelvar prasanthSelvar added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Jun 12, 2018
@mickaelandrieu
Copy link
Contributor Author

@PierreRambaud LGTM? ;)

@PierreRambaud PierreRambaud merged commit 1c6d820 into PrestaShop:1.7.4.x Jun 12, 2018
@mickaelandrieu mickaelandrieu deleted the improved-module-template-loader branch June 13, 2018 01:06
@PierreRambaud
Copy link
Contributor

Thanks @mickaelandrieu

@matks matks added the migration symfony migration project label Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.4.x Branch Bug Type: Bug migration symfony migration project QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants